gh-157242: Leave bytearray unchanged if resize() fails - #157243
Conversation
If bytearray.resize() or bytearray.take_bytes() fails, leave the bytearray unchanged. If PyBytesWriter_Resize() fails, leave the writer unchanged. Add a new internal _PyBytes_ResizeKeepOnError() function similar to _PyBytes_Resize() but leaves the bytes object unchanged on error.
vstinner
added
the
needs backport to 3.15
|
It would be nice if the "guaranteed no global" case was also used for the |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
No need to add a new parameter, it saves nothing. _PyBytes_Resize can be implemented via _PyBytes_ResizeKeepOnError.
I wonder if we can simply change the behavior of _PyBytes_Resize.
Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com>
For the in-place resize code path, no longer call _Py_ForgetReference() and _PyReftracerTrack() before PyObject_Realloc().
|
Please review the updated PR. I addressed reviews. @maurycy found a fix for the memmove() code path which worried me. I applied his suggestion and added a test.
Thanks for the advice. I reworked _PyBytes_Resize(): for the in-place resize code path, no longer call _Py_ForgetReference() and _PyReftracerTrack() before PyObject_Realloc(). Only call them on success. With this change, I was able to easy implement _PyBytes_Resize() with _PyBytes_ResizeKeepOnError(). |
Maybe Note: PR gh-156996 does fix PyBytes_FromStringAndSize() usage in bytearray. I don't try to replace this fix. |
|
bytearray.resize() and bytearray.take_bytes() have been fixed to no longer use a singleton: I merged main in my PR to get the PR gh-156996 fix. |
|
@maurycy: I modified resize() and take_bytes() to avoid memmove() usage if we would be unable to revert the bytesarray to its previous state on MemoryError. Does it look correct to you? I also added more tests injecting MemoryError. |
|
@vstinner: Thank you. I will take a look more carefully later today. |
| with self.assertRaises(MemoryError): | ||
| try: | ||
| _testcapi.set_nomemory(mem_error) | ||
| ba.take_bytes(5) |
There was a problem hiding this comment.
| ba.take_bytes(5) | |
| ba.take_bytes(to_take) |
if I understand the intent correctly
| #ifdef Py_TRACE_REFS | ||
| _Py_ForgetReference(v); | ||
| #endif | ||
| _PyReftracerTrack(v, PyRefTracer_DESTROY); |
There was a problem hiding this comment.
But v is now freed by PyObject_Realloc()? But ASan should've caught this?
| the number of bytes being removed in a resize is small; for large | ||
| size changes it may be better to just make a new bytes object as | ||
| _PyBytes_Resize will do a malloc + memcpy internally. */ | ||
| memmove(obj->ob_bytes, obj->ob_start, Py_SIZE(self)); |
There was a problem hiding this comment.
| memmove(obj->ob_bytes, obj->ob_start, Py_SIZE(self)); | |
| memmove(obj->ob_bytes, obj->ob_start, Py_SIZE(self)); | |
| obj->ob_bytes[Py_SIZE(self)] = '\0'; |
Do we care about this?
2026-09-10T14:51:02.756952000+0200 maurycy@gimel /Users/maurycy/work/cpython-pr157243 (pr157243 f51cba5?) % ./python.exe
Python 3.16.0a0 (heads/pr157243:f51cba57d3e, Sep 10 2026, 14:31:04) [Clang 21.0.0 (clang-2100.1.1.101)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import _testcapi, _testlimitedcapi
>>> ba = bytearray(b'0123456789')
>>> del ba[:3]
>>> ba
bytearray(b'3456789')
>>> try: _testcapi.set_nomemory(0); ba.resize(1024)
... except MemoryError: _testcapi.remove_mem_hooks()
...
>>> ba
bytearray(b'3456789')
>>> _testlimitedcapi.bytearray_asstring(ba, len(ba) + 1)
bytearray(b'34567897')
>>> _testlimitedcapi.bytearray_asstring(ba, len(ba) + 100)
bytearray(b'3456789789\x00\xfd\xfd\xfd\xfd\xfd\xfd\xfd\xfd\xdd\xdd\xdd\xdd\xdd\xdd\xdd\xdd\xdd\xdd\xdd\xdd\xdd\x00\x00\x00\x00\x00\x00\x008o\xfd\xfd\xfd\xfd\xfd\xfd\xfd\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xb0\xdd\xcd\x08\x01\x00\x00\x00@%\\\x03\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xfd\xfd\xfd')
>>> This is so subtle :(
There's a similar isue:
4 participants
If bytearray.resize() or bytearray.take_bytes() fails, leave the bytearray unchanged.
If PyBytesWriter_Resize() fails, leave the writer unchanged.
Add a new internal _PyBytes_ResizeKeepOnError() function similar to _PyBytes_Resize() but leaves the bytes object unchanged on error.